Skip to content

Roaster 71#71

Merged
gastaldi merged 1 commit intoforge:masterfrom
windmueller:ROASTER-71
Nov 20, 2016
Merged

Roaster 71#71
gastaldi merged 1 commit intoforge:masterfrom
windmueller:ROASTER-71

Conversation

@windmueller
Copy link
Copy Markdown

No description provided.

return this;
}

private void regenerateEqualsAndHashCode(final JavaClassSource source)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this method should belong to the Refactory class?

@windmueller
Copy link
Copy Markdown
Author

windmueller commented Nov 20, 2016

Yes, I thought about that, too. Moved it to Refactory now.

Additionally, the automatic detection of fields now skips non-final fields. The question is, should I stick with an automatic detection when any other method forces the user to specify the fields manually?

@gastaldi
Copy link
Copy Markdown
Member

That was my other thought. I think the optimal solution would be to make the method available for being called whenever the user requires it, passing the list of fields as arguments, wdyt?

@windmueller
Copy link
Copy Markdown
Author

That way you would have to specify those fields every time you want to change the name or type of a field, unless the fields are stored with the class object.

@gastaldi
Copy link
Copy Markdown
Member

Perhaps we would need to introduce some selector methods, like Fields.all() or .nonFinal() to specify them in the method signature of Refactory.generateEqualsAndHashcode. Not really sure what would be the best strategy here

*/
public static void regenerateEqualsAndHashCode(final JavaClassSource source)
{
List<FieldSource<JavaClassSource>> finalFieldsToUse = new LinkedList<FieldSource<JavaClassSource>>();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be parameterized

List<FieldSource<JavaClassSource>> finalFieldsToUse = new LinkedList<FieldSource<JavaClassSource>>();
for (PropertySource<JavaClassSource> property : source.getProperties())
{
if (property.getField() != null && property.getField().isFinal())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't check static fields, but I think we shouldn't assume anything. I like the idea of introducing field selectors or just passing the list of fields should be enough(the caller can use the streams API to filter accordingly)

@windmueller
Copy link
Copy Markdown
Author

The problem remains: How should the signature of setType look like? IMHO something like setType(Class<?> type, FieldSource<O>... fieldsForEqualsAndHashCode) does not look intuitive.

@gastaldi
Copy link
Copy Markdown
Member

I guess that the Refactory.regenerate method would have to be explicitly called by the user in this case.

@windmueller
Copy link
Copy Markdown
Author

Yes, but that would contradict the initial intention of this issue, which sounds to me like an implicit action when setType or setName are being called.

So, what should I do?

@gastaldi
Copy link
Copy Markdown
Member

IMHO setType and setName should not change its current behavior. We can't assume that the equals/hashCode implementation would rely on final fields, for example. Also, It doesn't seem to be possible to find where such field is used (in a method body for example) so I'm ok with making clear that it is the caller's responsibility to call Refactory.createEquals/createHashcode whenever a change to a field is performed.

Thus, I suggest we close the issue as Won't Fix and merge your code cleanup (which looks good) but don't introduce any different behavior.

Sounds good?

@windmueller
Copy link
Copy Markdown
Author

I agree. Just performed a force-push of the changes, including a Javadoc comment for setType.

@gastaldi gastaldi merged commit d9a17b8 into forge:master Nov 20, 2016
@gastaldi
Copy link
Copy Markdown
Member

Thank you! You rock!

@windmueller windmueller deleted the ROASTER-71 branch November 21, 2016 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants